GODRIVER-2388 Improved Bulk Write API. #1884

qingyang-hu · 2024-11-05T18:59:31Z

GODRIVER-2388
GODRIVER-3348
GODRIVER-3349
GODRIVER-3364

Summary

Improved Bulk Write API.

sort option will be added in GODRIVER-3285
handshakeError.json has not been added (blocked by GODRIVER-2191)

Background & Motivation

Refactor the (Operation).createWireMessage() to support the bulk write batching.

mongodb-drivers-pr-bot · 2024-11-05T19:02:03Z

API Change Report

./v2/mongo

compatible changes

(*Client).BulkWrite: added
ClientBulkWriteException: added
ClientBulkWriteResult: added
ClientDeleteManyModel: added
ClientDeleteOneModel: added
ClientDeleteResult: added
ClientInsertOneModel: added
ClientInsertResult: added
ClientReplaceOneModel: added
ClientUpdateManyModel: added
ClientUpdateOneModel: added
ClientUpdateResult: added
ClientWriteModels: added
NewClientDeleteManyModel: added
NewClientDeleteOneModel: added
NewClientInsertOneModel: added
NewClientReplaceOneModel: added
NewClientUpdateManyModel: added
NewClientUpdateOneModel: added

./v2/mongo/options

compatible changes

ClientBulkWrite: added
ClientBulkWriteOptions: added
ClientBulkWriteOptionsBuilder: added

./v2/x/mongo/driver

incompatible changes

(*Batches).AdvanceBatch: removed
(*Batches).ClearBatch: removed
(*Batches).Valid: removed
Batches.Current: removed
##NewCursorResponse: changed from func(ResponseInfo) (CursorResponse, error) to func(./v2/x/bsonx/bsoncore.Document, ResponseInfo) (CursorResponse, error)
Operation.Batches: changed from *Batches to interface{AdvanceBatches(n int); AppendBatchArray(dst []byte, maxCount int, maxDocSize int, totalSize int) (int, []byte, error); AppendBatchSequence(dst []byte, maxCount int, maxDocSize int, totalSize int) (int, []byte, error); IsOrdered() *bool; Size() int}
##Operation.ProcessResponseFn: changed from func(ResponseInfo) error to func(context.Context, ./v2/x/bsonx/bsoncore.Document, ResponseInfo) error
ResponseInfo.ServerResponse: removed

compatible changes

(*Batches).AdvanceBatches: added
(*Batches).AppendBatchArray: added
(*Batches).AppendBatchSequence: added
(*Batches).IsOrdered: added
(*Batches).Size: added
ExtractCursorDocument: added
ResponseInfo.Error: added

./v2/x/mongo/driver/session

incompatible changes

Client.RetryRead: removed
Client.RetryWrite: removed

./v2/x/mongo/driver/wiremessage

compatible changes

DocumentSequenceToArray: added

qingyang-hu · 2024-11-05T20:17:37Z

internal/integration/client_side_encryption_test.go

@@ -398,7 +398,7 @@ func TestClientSideEncryptionCustomCrypt(t *testing.T) {
 			"expected 0 calls to DecryptExplicit, got %v", cc.numDecryptExplicitCalls)
 		assert.Equal(mt, cc.numCloseCalls, 0,
 			"expected 0 calls to Close, got %v", cc.numCloseCalls)
-		assert.Equal(mt, cc.numBypassAutoEncryptionCalls, 2,
+		assert.Equal(mt, cc.numBypassAutoEncryptionCalls, 1,


We only call it once after the operation.go refactoring.

qingyang-hu · 2024-11-05T20:18:06Z

mongo/errors.go

+	// A top-level error that occurred when attempting to communicate with the server
+	// or execute the bulk write. This value may not be populated if the exception was
+	// thrown due to errors occurring on individual writes.
+	TopLevelError *WriteError


Cannot use Error as a field name because of the conflict with the conventional method name.

prestonvasquez · 2024-11-11T21:06:20Z

mongo/errors.go

@@ -612,6 +612,56 @@ func (bwe BulkWriteException) HasErrorCodeWithMessage(code int, message string)
 // serverError implements the ServerError interface.
 func (bwe BulkWriteException) serverError() {}

+// ClientBulkWriteException is the error type returned by ClientBulkWrite operations.
+type ClientBulkWriteException struct {


Should add a compile check for the bulk write exception:

var _ error = &ClientBulkWriteException{}

prestonvasquez · 2024-11-11T21:16:01Z

mongo/client.go

@@ -894,6 +892,86 @@ func (c *Client) createBaseCursorOptions() driver.CursorOptions {
 	}
 }

+// BulkWrite performs a client-level bulk write operation.
+func (c *Client) BulkWrite(ctx context.Context, models *ClientWriteModels,


This method should not panic when models=nil:

client, err := mongo.Connect() if err != nil { panic(err) } defer func() { _ = client.Disconnect(context.Background()) }() _, err = client.BulkWrite(context.Background(), nil) // Should not panic

Suggest propagating mongo.ErrNilValue and adding a unit test / extending the unified spec tests.

prestonvasquez · 2024-11-11T21:17:32Z

mongo/client.go

+// BulkWrite performs a client-level bulk write operation.
+func (c *Client) BulkWrite(ctx context.Context, models *ClientWriteModels,
+	opts ...options.Lister[options.ClientBulkWriteOptions]) (*ClientBulkWriteResult, error) {
+	// TODO: Remove once DRIVERS-2888 is implemented.


The corresponding GODRIVER ticket should be included in the TODO:

Suggested change

// TODO: Remove once DRIVERS-2888 is implemented.

// TODO(GODRIVER-3403): Remove after support for QE with Client.bulkWrite.

prestonvasquez · 2024-11-11T21:26:23Z

mongo/client_bulk_write_models.go

+}
+
+// AppendInsertOne appends ClientInsertOneModels.
+func (m *ClientWriteModels) AppendInsertOne(database, collection string, models ...*ClientInsertOneModel) *ClientWriteModels {


Suggest abstracting the Append* methods:

type clientBulkWriteModel interface { ClientInsertOneModel } // appendModels is a helper function to append models to ClientWriteModels. func appendModels[T clientBulkWriteModel](m *ClientWriteModels, database, collection string, models []*T) *ClientWriteModels { if m == nil { m = &ClientWriteModels{} } for _, model := range models { m.models = append(m.models, clientWriteModel{ namespace: fmt.Sprintf("%s.%s", database, collection), model: model, }) } return m }

prestonvasquez · 2024-11-11T21:30:03Z

mongo/client_bulk_write_models.go

+}
+type clientWriteModel struct {
+	namespace string
+	model     interface{}


Should we add stronger type constraints to this?

type clientBulkWriteModel interface { ClientInsertOneModel // etc. } type clientWriteModel struct { namespace string model clientBulkWriteModel }

I don't think we need an additional abstraction for an un-exported struct.

prestonvasquez · 2024-11-11T22:37:45Z

mongo/results.go

@@ -13,6 +13,51 @@ import (
 	"go.mongodb.org/mongo-driver/v2/x/mongo/driver/operation"
 )

+// ClientBulkWriteResult is the result type returned by a client-level BulkWrite operation.
+type ClientBulkWriteResult struct {


Why not include Acknowledged in the spirit of GODRIVER-2821 ?

prestonvasquez · 2024-11-11T22:39:30Z

mongo/results.go

+}
+
+// ClientInsertResult is the result type returned by a client-level bulk write of InsertOne operation.
+type ClientInsertResult struct {


Suggest naming this ClientBulkWriteInsertOneResult to avoid confusion.

prestonvasquez · 2024-11-11T22:39:46Z

mongo/results.go

+}
+
+// ClientUpdateResult is the result type returned from a client-level bulk write of UpdateOne, UpdateMany, and ReplaceOne operation.
+type ClientUpdateResult struct {


Suggest naming this to ClientBulkWriteUpdateResult

prestonvasquez · 2024-11-11T22:40:14Z

mongo/results.go

+}
+
+// ClientDeleteResult is the result type returned by a client-level bulk write DeleteOne and DeleteMany operation.
+type ClientDeleteResult struct {


Suggest naming this to ClientBulkWriteDeleteResult

prestonvasquez · 2024-11-11T22:48:31Z

mongo/errors.go

+}
+
+// Error implements the error interface.
+func (bwe ClientBulkWriteException) Error() string {


This function doesn't return an error if the write is unacknowledged. The specifications required that users be able to discern whether a BulkWriteResult contains acknowledged results. Either return an error indicating an unacknowledged result, or update ClientBulkWriteResult in the spirit of GODRIVER-2821.

You can test this with the following:

package main import ( "context" "go.mongodb.org/mongo-driver/v2/bson" "go.mongodb.org/mongo-driver/v2/mongo" "go.mongodb.org/mongo-driver/v2/mongo/options" "go.mongodb.org/mongo-driver/v2/mongo/writeconcern" ) func main() { client, err := mongo.Connect() if err != nil { panic(err) } defer func() { _ = client.Disconnect(context.Background()) }() pairs := &mongo.ClientWriteModels{} insertOneModel := mongo.NewClientInsertOneModel().SetDocument(bson.D{{"x", 1}}) opts := options.ClientBulkWrite().SetWriteConcern(writeconcern.Unacknowledged()).SetOrdered(false) pairs = pairs.AppendInsertOne("db", "k", insertOneModel) _, err = client.BulkWrite(context.Background(), pairs, opts) // Should not panic if err != nil { panic(err) } }

Is there a unified spec test that covers this case? If not we should add one / add an integration test.

qingyang-hu added 2 commits November 4, 2024 16:55

GODRIVER-2388 Improved Bulk Write API.

1309416

GODRIVER-2388 implementation

4d88edf

mongodb-drivers-pr-bot bot added the priority-3-low Low Priority PR for Review label Nov 5, 2024

qingyang-hu commented Nov 5, 2024

View reviewed changes

qingyang-hu marked this pull request as ready for review November 5, 2024 20:18

qingyang-hu requested review from matthewdale and prestonvasquez November 5, 2024 20:18

qingyang-hu added priority-2-medium Medium Priority PR for Review and removed priority-3-low Low Priority PR for Review labels Nov 5, 2024

qingyang-hu mentioned this pull request Nov 5, 2024

GODRIVER-3285 Allow update to supply sort option. #1797

Open

prestonvasquez requested changes Nov 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GODRIVER-2388 Improved Bulk Write API. #1884

GODRIVER-2388 Improved Bulk Write API. #1884

qingyang-hu commented Nov 5, 2024

mongodb-drivers-pr-bot bot commented Nov 5, 2024

qingyang-hu Nov 5, 2024

qingyang-hu Nov 5, 2024

prestonvasquez Nov 11, 2024

prestonvasquez Nov 11, 2024

prestonvasquez Nov 11, 2024

prestonvasquez Nov 11, 2024

prestonvasquez Nov 11, 2024

qingyang-hu Nov 13, 2024

prestonvasquez Nov 11, 2024

prestonvasquez Nov 11, 2024

prestonvasquez Nov 11, 2024

prestonvasquez Nov 11, 2024

prestonvasquez Nov 11, 2024

prestonvasquez Nov 11, 2024

prestonvasquez Nov 11, 2024

	// TODO: Remove once DRIVERS-2888 is implemented.
	// TODO(GODRIVER-3403): Remove after support for QE with Client.bulkWrite.

GODRIVER-2388 Improved Bulk Write API. #1884

Are you sure you want to change the base?

GODRIVER-2388 Improved Bulk Write API. #1884

Conversation

qingyang-hu commented Nov 5, 2024

Summary

Background & Motivation

mongodb-drivers-pr-bot bot commented Nov 5, 2024

API Change Report

./v2/mongo

compatible changes

./v2/mongo/options

compatible changes

./v2/x/mongo/driver

incompatible changes

compatible changes

./v2/x/mongo/driver/session

incompatible changes

./v2/x/mongo/driver/wiremessage

compatible changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment